Repository navigation
Conversation
- HttpClientResponseData: copy each response header once from the enumerated values instead of re-looking every key up with TryGetValues; build GetHeaderNames lazily from the dictionary and answer IsHeaderPresent from the dictionary instead of a second HashSet. - User-Agent: reuse the last generated string per HttpHandler when the ordered components and feature ids are unchanged; merge request-level UA details without the intermediate join/enumerator; avoid the UTF-8 byte[] in the 1 KB truncation check. - HttpHandler: reuse the previously parsed request Uri when the composed URL string (and the canonicalization setting) is unchanged (non-BCL); skip attribute building/boxing in RecordHttpTelemetryData when the meter/tracer providers are the built-in no-op ones; skip RequestSize boxing when RequestMetrics is disabled. - HttpWebRequestMessage: use the shared HttpMethod instances for the standard methods; enumerate concrete Dictionary headers without boxing. HttpAdapterTests expects the runtime's own exception type for HttpMethod(null): netcoreapp3.1 throws ArgumentException where net472 and .NET 5+ throw ArgumentNullException, and GetHttpMethod passes null through unchanged.
b79c2c1 to
e55ee3e
Compare
…only header collisions
Merge near-duplicate tests into data rows, assert literal User-Agent values, and use the shared RecordingTelemetry.cs helpers. The response-header test still compares against the previous copy algorithm.
Only tests passed a cached entry; HttpHandler.GetUserAgent already does the matching through UserAgentDetails.Matches, so the method no longer takes one.
| } | ||
|
|
||
| var generated = userAgentDetails.CreateGeneratedUserAgent(); | ||
| var slot = (Interlocked.Increment(ref _nextUserAgentCacheSlot) - 1) & (UserAgentCacheSize - 1); |
There was a problem hiding this comment.
Curious, did you measure this cache on its own? The PR description has one number for all five changes, and the lookup (seems to) walk two HashSets per entry (so I'm not sure the complexity - Volatile / Interlocked / etc. - is worth it...)
There was a problem hiding this comment.
On its own, in a microbenchmark, a cache hit costs 30 ns against 413 ns to rebuild the string, so it saves about 0.4 µs per call. With the ops/CPU-sec harness I measured it together with the Uri reuse: on six small cases (9 interleaved rounds each), removing both raised the HttpHandler request stage by 0.4 to 3.5 µs per call, added about 1.1 KB of allocations per call, and dropped ops/CPU-sec by 2.1% (geometric mean). The slots are only written on a miss, so I'd like to keep it.
| /// Returns the same Uri as <see cref="AmazonServiceClient.ComposeUrl(IRequest)"/>, reusing the previously | ||
| /// parsed Uri when the composed URL string is unchanged. | ||
| /// </summary> | ||
| internal Uri ComposeRequestUrl(IRequest request) |
There was a problem hiding this comment.
Similar question here: did you measure the Uri reuse on its own? The URL string is still built per call, so this only skips the Uri parse, and it costs two new ComposeUrl helpers, a ComposedUri class and three #if branches.
There was a problem hiding this comment.
On its own, in a microbenchmark, skipping the Uri parse saves 0.25 to 0.3 µs per call. With the ops/CPU-sec harness I measured it together with the User-Agent cache: removing both cost 2.1% ops/CPU-sec on six small cases and about 1.1 KB more allocated per call in the HttpHandler request stage. So I'd like to keep it.
| @@ -530,13 +548,25 @@ public void ConfigureRequest(IRequestContext requestContext) | |||
| /// <param name="headers">A dictionary of header names and values.</param> | |||
| public void SetRequestHeaders(IDictionary<string, string> headers) | |||
| { | |||
| foreach (var kvp in headers) | |||
| // Enumerate the concrete type when possible to avoid boxing the enumerator. | |||
There was a problem hiding this comment.
Was there a measurable difference from this? It's one boxed enumerator per request, and we now have two copies of the loop.
There was a problem hiding this comment.
No measurable difference. It saved 61 ns and 56 bytes per request in a microbenchmark and nothing visible end to end, so I went back to the single loop in 13d1447.
|
|
||
| return string.Empty; | ||
| return values.FirstOrDefault(); |
There was a problem hiding this comment.
Similar to the other comment about dictionary / duplicating loops: I'd be surprised if this had a significant difference... (I'd expect FirstOrDefault to already be optimized)
There was a problem hiding this comment.
This one does make a difference. On a 9-header S3 response, FirstOrDefault adds about 450 ns (50 ns per header) in a microbenchmark and 0.3 µs end to end. The larger saving is taking the value from the enumerated pair instead of looking each key up again with TryGetValues: 2.3 µs per response end to end, the largest single piece of this PR.
There was a problem hiding this comment.
Sorry for the broken record, but same comment as on the other two PRs: this is a lot of new test code for the change. We already have UserAgentTests and HttpHandlerTests, so new cases should go there.
RecordingTelemetry.cs is also the same file as in #4559 and ReferenceCopyHeaders just re-implements the old code as the expected value - a few explicit expected headers would be easier to follow.
There was a problem hiding this comment.
Moved the cases into UserAgentTests and HttpHandlerTests, dropped the ones that repeat existing coverage, and removed HttpAdapterTests.cs and RecordingTelemetry.cs in 13d1447 and 46bb8ab. The test diff went from 712 to 293 lines. The header test now checks explicit expected headers instead of ReferenceCopyHeaders, and the telemetry test uses two small private providers.
…nd HttpHandlerTests
…sing-header value
Description
enumerated values instead of re-looking every key up with TryGetValues;
build GetHeaderNames lazily from the dictionary and answer
IsHeaderPresent from the dictionary instead of a second HashSet.
ordered components and feature ids match exactly (a client alternating
sync and async calls, or operations with different feature ids, keeps one
entry per shape); merge request-level
UA details without the intermediate join/enumerator; avoid the UTF-8
byte[] in the 1 KB truncation check.
URL string (and the canonicalization setting) is unchanged (non-BCL);
skip attribute building and boxing in RecordHttpTelemetryData when the
trace span and meter provider are the SDK's no-op ones; skip RequestSize boxing when RequestMetrics
is disabled.
standard methods.
HttpAdapterTests expects the runtime's own exception type for
HttpMethod(null): netcoreapp3.1 throws ArgumentException where net472 and
.NET 5+ throw ArgumentNullException, and GetHttpMethod passes null through
unchanged.
Motivation and Context
Measurement: Numbers come from the serde ops/CPU-sec runner (
sdk/test/Performance/SerdeBenchmarks,--cpu-time, stub transport), geomean of 71 cases, 3 interleaved runs. Instances: m7i.metal-24xl (x86) and m7g.metal (ARM64). Settings: net8.0, 20,000-iteration warmup, measured ondevelopment8956121, which has the same code as this branch's base for these files. Each change is measured alone against that base. Both metal runs measured an earlier revision of this PR that kept one User-Agent string per client instead of four. Onfeature/4.1(337f357, net10.0, 2,000-iteration warmup, 3 interleaved rounds, same instances) this change gained +5.8% on x86 and +5.1% on ARM. The final run at the harness defaults (net10.0, 100,000 warmup, stub and https) is tracked separately.Clients that alternate request shapes. A cache miss costs no more than today's code. In a development-host run (one client, alternating
GetObjectkeys and/or User-Agent components, stub transport, 9 interleaved reps), every alternating pattern was 6.6–7.8% cheaper per call thandevelopment, against 7.4% when nothing alternates. The −3.4% onawsQuery_GetMetricDataResponse_Sin thedevelopmentrun didn't reproduce onfeature/4.1(+4.9% x86, +3.9% ARM), where the worst of the 71 cases was −0.9% on x86, within the noise.Testing
HttpHandlerTestsandUserAgentTests: response headers against explicit expected values (multi-value, empty, invalid, case-only collision, cached header names, missing header);Urireuse and canonicalization changes;RequestSizerecorded only when request metrics are enabled; standardHttpMethodreuse; HTTP metrics and span attributes with custom providers; User-Agent reuse with alternating shapes and with more shapes than cache entries.feature/4.160dfa62.Breaking Changes Assessment
None to the public API.
HttpClientResponseData(Amazon.Runtime.Internal.Transform) no longer throws when a response header and a content header differ only in case (the response header's value is kept), andIsHeaderPresentreturns false instead of throwingNullReferenceExceptionon event-stream message data.Credit
@dscpinheiro independently arrived at the same response-side change in 9b07f37: copying each response header once and answering
IsHeaderPresent/GetHeaderNamesfrom the header dictionary.Dry-runs
c30c786d-7644-4362-8263-158a3ee4e39a1f4ee6b1-6803-4264-9eeb-e94bad3c244fChecklist
License